Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sparseconvert: copy falls back to copymutable for non-sparse SubArray #32249

Closed
wants to merge 1 commit into from
Closed

sparseconvert: copy falls back to copymutable for non-sparse SubArray #32249

wants to merge 1 commit into from

Conversation

KlausC
Copy link
Contributor

@KlausC KlausC commented Jun 5, 2019

Fixes #32213

@fredrikekre
Copy link
Member

Why is this method needed at all?

@KlausC
Copy link
Contributor Author

KlausC commented Jun 6, 2019

@fredrikekre, in the case of views of sparse matrices, there is an essential performance improvement compared to the generic Base.copymutable implementation possible (for example factor 800 for n=10^3, nnz=10^4).

julia> n = 1000
1000
julia> A = sprand(n, n, 10^4/n^2)
1000×1000 SparseMatrixCSC{Float64,Int64} with 10166 stored entries:
...
julia> B = view((A), 1:999, 1:999);

julia> @btime copy($B);
  56.902 μs (6 allocations: 166.64 KiB)

julia> @btime Base.copymutable($B);
  46.470 ms (8 allocations: 325.92 KiB)

@KristofferC
Copy link
Member

Instead of type piracing this on all arrays, can't it be restricted to only apply to the sparse arrays we have?

@KlausC
Copy link
Contributor Author

KlausC commented Jun 6, 2019

@KristofferC, it could be restricted to a finite number of cases by union or unionall type restrictions, but not to the complete infinite set of nested wrapped sparse matrices. That was the intention of merged PR #30552, which includes the now fixed definition of copy.
Because I had the impression, that the approach in this PR is somhow smelly, I made an alternative approach in PR #31563, which would allow to identify the mentioned types by an additional type parameter and amendment of the type hierarchy (AbstractArray :> AbstractWrappedArray :> SubArray). Once the second PR, or similar functionality, would be accepted, the cases of type piracy in the temporary approach could be avoided completely.

@fredrikekre
Copy link
Member

@fredrikekre, in the case of views of sparse matrices, there is an essential performance improvement compared to the generic Base.copymutable implementation possible

Yea, I get that, but (i) I don't see how that relates to the changes in #30552. Where is copy called? and (ii) do we really need a special copy method for views of transposes/adjoints/etc of sparsematrices?

@KlausC
Copy link
Contributor Author

KlausC commented Jun 8, 2019

@fredrikekre, I try to remember my motivation to propose this method as part of #30552.

(i) I don't see how that relates to the changes in #30552. Where is copy called?

I recognized, that there are specialized methods of copy for most other wrapper types, except SubArray, which make use of the sparsity of its argument. I wanted to extend the performance improvements to all kinds of wrapped sparse matrices. copy is called by the user and not needed for the implementation of the rest of #30552.

(ii) do we really need a special copy method for views of transposes/adjoints/etc of sparsematrices?

I don't know. It is just an attempt to prevent the user from unexpectedly worse operation times.

@fredrikekre
Copy link
Member

fredrikekre commented Jun 8, 2019

Okay. Can I suggest that we completely drop this copy method for now and we can open a PR/issue where copy(::SubArray) is discussed? Seems like the most sane option since it was an unrelated change and presumably no-one really reviewed.

Edit: #32266

@KlausC
Copy link
Contributor Author

KlausC commented Jun 8, 2019

understood.

@KristofferC KristofferC closed this Jun 9, 2019
@KlausC KlausC deleted the krc/sparseconvert branch June 10, 2019 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1.2: SparseArrays commits type piracy in override of copy
3 participants